To test the PR using lua_ls you can have a project with the following structure

test_project/
|- main.lua
|- something.lua
|- .git

.git is an empty file, I simply use it so `lua_ls` has a `root_marker`

with the following content

-- main.lua
local something = require("something").something

something()

-- something.lua
local M = {}

M.something = function()
	print("something")
end

return M

you can then nvim main.lua, : lua MiniFiles.open()m rename something.lua to something_else.lua and accept the changes with =. Neovim should prompt you for confirmation before updating the import statement on main.lua to local something = require("something_else").something

The tests errors seem to go away if I comment out

and

But I'm not sure why.

In particular, the problematic lines seem to be

and

changing them to

local clients = {}

seems to also make the test pass.

Are these tests specially time sensitive? I can't think of another reason of why they would be failing

Are these tests specially time sensitive? I can't think of another reason of why they would be failing

I don't think so. Probably, some logic has been broken. Didn't read too much into the code.

• the code should make some considerations to support older Neovim versions (vim.glob.to_lpeg has a bug prior to 0.11 that requires manually sorting items inside brackets as a workaround, client:request_sync/client:notify are not methods prior to 0.11, etc)

Restricting this functionality to only Neovim<0.11 is fine.

• I though about checking the user permissions on a given directory to further test if the file operations will succeed, but the file permissions semantics for these kind of operations seem to be different in Linux and Windows (and, as far as I can tell, libuv does not offer an abstraction on top of it. Maybe fs_stat related function would work on Windows with the same semantics, I haven't tested it yet)

I am okay with these checks for willXxx requests be on the "best effort" basis. It is the fault of the LSP specifications to have designed such a questionable (i.e. I don't understand why it is like this) approach :)

I'll also take a look into how to better handle the "close on lost focus" in 'mini.files'. It will probably have to be "don't close if inside non-normal buffer in a floating window" type of exception.

I'll also take a look into how to better handle the "close on lost focus" in 'mini.files'. It will probably have to be "don't close if inside non-normal buffer in a floating window" type of exception.

Okay, I've looked into this. Going the "don't close explorer if in floating window" goes against some of the use cases that originally prompted the tracking of lost focus. Like "I want to nvim . and open file picker" and in general opening a file picker when inside an explorer.

I think the best compromise here would be to temporarily adjust vim.ui.select() to set a temporary flag to ignore tracking lost cursor.

Something like this (as of the current state of this PR) seems to work:

diff --git a/lua/mini/files.lua b/lua/mini/files.lua
index 304e895f..fce1bbb7 100644
--- a/lua/mini/files.lua
+++ b/lua/mini/files.lua
@@ -1540,7 +1540,7 @@ end
 H.explorer_track_lost_focus = function()
   local track = vim.schedule_wrap(function()
     local ft = vim.bo.filetype
-    if ft == 'minifiles' or ft == 'minifiles-help' then return end
+    if H.skip_track_lost_focus or ft == 'minifiles' or ft == 'minifiles-help' then return end
     local cur_win_id = vim.api.nvim_get_current_win()
     MiniFiles.close()
     pcall(vim.api.nvim_set_current_win, cur_win_id)
@@ -2753,6 +2753,15 @@ H.lsp_will_fs_do = function(action, params)
   local full_method = 'workspace/' .. method .. 'Files'
   local clients = vim.lsp.get_clients({ method = full_method })
 
+  local ui_select_orig = vim.ui.select
+  vim.ui.select = function(items, opts, on_choice)
+    H.skip_track_lost_focus = MiniFiles.get_explorer_state() ~= nil
+    ui_select_orig(items, opts, function(...)
+      H.skip_track_lost_focus, vim.ui.select = nil, ui_select_orig
+      on_choice(...)
+    end)
+  end
+
   -- TODO(TheLeoP): configurable timeout
   local timeout = 1000
   for _, client in ipairs(clients) do

One thing to extra consider is whether there will be any side effects if vim.ui.select() is not called as a result of the LSP request.

Thinking about it more, maybe it is a good idea to have this kind of mock in general for the whole duration of the explorer. Will also work nicely with something like 'mini.snippets' (which can use vim.ui.select to show matched snippets). Hmm... Need to think a bit more.

Thinking about it more, maybe it is a good idea to have this kind of mock in general for the whole duration of the explorer. Will also work nicely with something like 'mini.snippets' (which can use vim.ui.select to show matched snippets). Hmm... Need to think a bit more.

Yeah, ignoring tracking lost focus while vim.ui.select() (or vim.ui.input()) is active is probably a good idea anyway. Here is my current WIP solution.

I tried it with this PR and:

• With using 'mini.pick' for vim.ui.select renaming a single file works (with lua_ls), renaming two - does not. Because it tries to spawn two 'mini.pick' windows in parallel, which is not possible and there are cryptic errors. I think using vim.ui.select() for this kind of stuff in core is a mistake (especially with coroutine kind of magic). But at least this is a one more argument for bulk processing LSP requests.
• With 'snacks.nvim' for vim.ui.select renaming even a single just doesn't work. No LSP related adjustments are made.

Thanks for your feedback! I'll be a bit busy today and tomorrow, so I'll address your comments and I'll keep working on this PR on Sunday

• With using 'mini.pick' for vim.ui.select renaming a single file works (with lua_ls), renaming two - does not. Because it tries to spawn two 'mini.pick' windows in parallel, which is not possible and there are cryptic errors. I think using vim.ui.select() for this kind of stuff in core is a mistake (especially with coroutine kind of magic). But at least this is a one more argument for bulk processing LSP requests.

Soo... I spent some time investigating in the hopes of opening an issue in neovim/neovim to drop the vim.ui.select() + coroutine solution. Key takeaways:

• The reason for this approach was feat(lsp): run handler in coroutine to support async response neovim/neovim#21026.
• Many consecutive calls that make window/showMessageRequest (which is for LuaLS is done after workspace/didRenameFiles) work with built-in vim.ui.select() and some simple async one. But it doesn't work with all currently popular vim.ui.select() implementations: 'mini.pick', 'snacks.picker', 'fzf-lua'. My guess is that all of them use coroutines themselves to have optimized behavior.
• I don't think Neovim core is going to change anything since it was a result of a wider change of running with coroutines. Hence I am not keen on spending countless hours arguing that for core.
• The best way forward looks to me like:
  • Make only a single LSP request/notify per method+sync pair. I.e. aggregate create, rename, delete actions and advertise them to the LSP servers once per synchronization. This will minimize issues with all vim.ui.select implementations.
  • Figure out a way for 'mini.pick' to have two and more pickers "in the queue". Maybe only for vim.ui.select(). Right now it throws an error, which is not nice. This should now work on the latest main.

Repro steps for what I did:

• Create even more minimal Lua project:

  Test Lua project

  'main.lua':

  local uuu = require('uuu')
  local vvv = require('vvv')

  'uuu.lua':

  return 'uuu'

  'vvv.lua':

  return 'vvv'

• Create an 'init.lua' with a separate NVIM_APPNAME:

  ~/.config/nvim-repro/init.lua

  -- vim.ui.select() implementation =============================================
  
  -- -- Custom implementation
  -- local n = 0
  -- vim.ui.select = function(items, opts, on_choice)
  --   local win_id
  --   local width = math.ceil(0.25 * vim.o.columns)
  --   local height = math.min(vim.o.lines, #items)
  --
  --   local buf_id = vim.api.nvim_create_buf(false, true)
  --   local lines = vim.tbl_map(opts.format_item or tostring, items)
  --   vim.api.nvim_buf_set_lines(buf_id, 0, -1, false, lines)
  --
  --   n = n + 1
  --   local win_opts = { relative = 'editor', row = n, col = n, height = height, width = width, border = 'single' }
  --   win_id = vim.api.nvim_open_win(buf_id, true, win_opts)
  --   vim.api.nvim_win_set_cursor(win_id, { 1, 0 })
  --
  --   -- Choose and close
  --   local close = function(idx)
  --     vim.api.nvim_win_close(win_id, true)
  --     vim.api.nvim_buf_delete(buf_id, { force = true })
  --     on_choice(items[idx], idx)
  --   end
  --
  --   local choose = function() close(vim.api.nvim_win_get_cursor(win_id)[1]) end
  --   vim.keymap.set('n', '<CR>', choose, { buffer = buf_id })
  --
  --   local quit = function() close() end
  --   vim.keymap.set('n', '<Esc>', quit, { buffer = buf_id })
  -- end
  
  -- From 'mini.pick'
  vim.pack.add({ 'https://github.com/nvim-mini/mini.nvim' })
  require('mini.pick').setup()
  
  -- -- From 'fzf-lua'
  -- vim.pack.add({ 'https://github.com/ibhagwan/fzf-lua' })
  -- require('fzf-lua').setup()
  -- require('fzf-lua').register_ui_select()
  
  -- -- From 'snacks.nvim'
  -- vim.pack.add({ 'https://github.com/folke/snacks.nvim' })
  -- require('snacks').setup({ picker = { enable = true } })
  
  -- Helpers for `workspace/didRenameFiles` notifications =======================
  _G.start_luals = function()
    local config = { name = 'lua-ls', cmd = { 'lua-language-server' }, root_dir = vim.fn.getcwd() }
    _G.client_id = vim.lsp.start(config)
  end
  
  local make_didrename_params = function(from_name, to_name)
    local cwd = vim.fs.abspath(vim.fn.getcwd())
    local old = 'file://' .. vim.fs.joinpath(cwd, from_name)
    local new = 'file://' .. vim.fs.joinpath(cwd, to_name)
    return { files = { { oldUri = old, newUri = new } } }
  end
  
  _G.make_one_notify = function()
    local client = vim.lsp.get_client_by_id(_G.client_id)
    client:notify('workspace/didRenameFiles', make_didrename_params('uuu.lua', 'uuu2.lua'))
  end
  
  _G.make_two_notify = function()
    local client = vim.lsp.get_client_by_id(_G.client_id)
    client:notify('workspace/didRenameFiles', make_didrename_params('uuu.lua', 'uuu2.lua'))
    client:notify('workspace/didRenameFiles', make_didrename_params('vvv.lua', 'vvv2.lua'))
  end
  
  _G.make_two_notify_simul = function()
    local cwd = vim.fs.abspath(vim.fn.getcwd())
    local old_1 = 'file://' .. vim.fs.joinpath(cwd, 'uuu.lua')
    local new_1 = 'file://' .. vim.fs.joinpath(cwd, 'uuu2.lua')
    local old_2 = 'file://' .. vim.fs.joinpath(cwd, 'vvv.lua')
    local new_2 = 'file://' .. vim.fs.joinpath(cwd, 'vvv2.lua')
    local params = { files = { { oldUri = old_1, newUri = new_1 }, { oldUri = old_2, newUri = new_2 } } }
  
    local client = vim.lsp.get_client_by_id(_G.client_id)
    client:notify('workspace/didRenameFiles', params)
  end

• NVIM_APPNAME=nvim-repro nvim -- main.lua uuu.lua vvv.lua from the root of the test Lua project. Assumes lua-language-server is installed systemwide.

• :lua start_luals() and wait for it to load (diagnostic hints will be shown in sign column and statusline).

• :lua make_one_notify() and accept. It works. Revert its changes.

• :lua make_two_notify_simul() and accept. It works. Revert its changes.

• :lua make_two_notify(). It works only for built-in and custom minimal vim.ui.select(), but not for 'mini.pick' (throws error), 'fzf-lua' (only shows one selection window and acts only on one notification), 'snacks.picker' (just does nothing).

The bulk LSP request/notifications should be working now. I left them on separated commits to make reviewing a bit easier.

Let me know if you have any more feedback regarding the implementation. After that is done, I'll focus on creating some tests. What would be your preference for testing this? My guess is that it would be to mock a language server.

I can take a look at mini.pick later to see if I can do something to allow it to queue vim.ui.select calls.

• Figure out a way for 'mini.pick' to have two and more pickers "in the queue". Maybe only for vim.ui.select(). Right now it throws an error, which is not nice.

This should now work on the latest main. As well as calling vim.ui.select() or vim.ui.input() when inside 'mini.files' explorer.

I just tested the latest changes on main and they work perfectly. Thanks!

To spare both of us the back and forth about mine vague "make less nesting" suggestion, I've pushed what I think is a reasonable improvement (4b300bc). It did increase line count by 12 lines, but this is probably due to comments, new robustness checks, and performance improvements (mostly by avoiding doing things when they are not needed). This should work okay now.

Please double check the new code. If you notice an incorrect behavior, please point that out. After that - squash all commits before it.

Now the less fun part - tests. Yes, I'd like this to be done by an explicit new mocked LSP server (there are 'tests/mock-lsp' examples for reference). It essentially should test:

• All LSP actions are done in full when they are expected to be done. With appropriate parameters at appropriate time.
• No LSP actions are done if they are expected to be not done (like not fit any kind of filter, etc.).
• Works with several active LSP servers.

For the didXxx notifications, would you prefer the mock LSP to react to them by:

• triggering a window/showMessageRequest request to the client and then sending a workspace/applyEdit request to the client (like lua_ls does)
• directly send a workspaced/applyEdit request
• some other option, like doing the changes inside of the mock LSP server by using the Neovim API directly

Also, for the tests with multiple LSP servers attached, you want them to be different servers that react to the wilXxx/didXxx LSP hooks, right?

I still have to implement the tests for the didXxx notifications and multiple LSP clients, but let me know what you think about the other two tests so far.

For the failing tests, I need to regenerate the snapshot and update the expected highlight groups, right? Also, is the mock directory the correct location for the will.lua file? I'm not confident in my understanding of the directory structure.

I also caught an error with my previous assumptions regarding ignoreCase while creating the tests and fixed it in the last commit :p

For the didXxx notifications, would you prefer the mock LSP to react to them by:

Whatever you think will cover real world usage better without accounting for rare cases that are not required by the spec. My guess would be that window/showMessageRequest is one of those things. Sending an workspaced/applyEdit may as well also be one of those things. As far as I understand the spec, the notification just needs to be recorded by the server and later it needs to decide to do whatever it wants to do. So maybe even only testing that notification was received with proper data is enough.

For the failing tests, I need to regenerate the snapshot and update the expected highlight groups, right?

If they are the result of the adding new entries to 'tests/dir-files' (which they indeed look like), then yeah.

Also, is the mock directory the correct location for the will.lua file? I'm not confident in my understanding of the directory structure.

As this is a file that is used within 'mini.files' tests, then it should be inside a 'tests/dir-files' directory. Beyond that it is on "whatever feels right" basis. I'd put this in the 'tests/dir-files/lsp-files' directory.

Also, for the tests with multiple LSP servers attached, you want them to be different servers that react to the wilXxx/didXxx LSP hooks, right?

Whatever reasonably covers the "there are multiple LSP clients attached to the buffer" cases. Maybe a separate test case that has several servers attached, each with different capabilities? Like to test that if the server doesn't support a method, then there is no request done for it.

It took me longer than I expected, but the tests cover a bunch more use cases now. So, I marked the PR as ready for review.

I even ended up opening neovim/neovim#39099 while looking into how to send server to client requests from an in-memory LSP :p.

Sorry for all the back and forth, btw.

I'm not sure why the tests on ubuntu with Neovim 0.10.4 are failling. It seems to be hitting neovim/neovim#35550 , but that issue hasn't been solved as far as I can tell. So, I'm not sure why it would only happen on 0.10.4.

I'm also not sure why Windows is not triggering the willCreate hook for the file aaa. Will take a look tomorrow

Sorry for all the back and forth, btw.

No need to apologize. That the nature of the PR review and, if anything, it should be me who needs to apologize :)

I'm not sure why the tests on ubuntu with Neovim 0.10.4 are failling. It seems to be hitting neovim/neovim#35550 , but that issue hasn't been solved as far as I can tell. So, I'm not sure why it would only happen on 0.10.4.

Does the feature even work on Neovim 0.10 or is it only test? If not, I am fine with only enabling this on Neovim>=0.11 (i.e. -- TODO: Remove after compatibility with Neovim=0.10 is dropped, if vim.fn.has('nvim-0.11') == 0 then H.lsp_fs_hook = function() end end.

I'm also not sure why Windows is not triggering the willCreate hook for the file aaa. Will take a look tomorrow

Sorry, I have no clue either. Maybe **/{aaa,bbb} glob doesn't work there like on Unix?

I am still thinking about the best way to have this configurable. Do you think something like config.options.lsp_integration = true would be enough? Or should it be full-blown config.lsp = { enable = true, timeout = 1000 } (as in 'oil.nvim')?

I prefer the first option (config.options.lsp_integration = true), I don't think many people would like to customize the timeout. But, the second option seems more future proof in case other LSP related options need to be added eventually.

After that last change the tests at least seem to be running on 0.10.4. I'll keep investigating tomorrow, I also want to debug the Windows failure on my Windows PC.

I've pushed some tweaks to the latest changes. Including a change in how client is requested on Neovim<0.11. Tests still fail (differently), but my guess is that this needs adjustments in mocks.

I also removed the TODO about showing an error to the user. This is not how other LSP aware modules work (like 'mini.completion'), they silently ignore errors. If the behavior is to be changed, then It should be consistent across all modules.

I prefer the first option (config.options.lsp_integration = true), I don't think many people would like to customize the timeout. But, the second option seems more future proof in case other LSP related options need to be added eventually.

Whether it needs to be future proof is the whole point :)

Let's make it more concise then: config.options.lsp_timeout = 1000 with value 0 meaning no LSP integration. The 'oil.nvim' also has the "autosave" option, but I don't think I like this behavior (requiring an explicit save from the user seems less surprising).

The relevant section in 'MAINTAINING.md' will be a helpful checklist here.